-
Notifications
You must be signed in to change notification settings - Fork 2.3k
MariaDB Metadata skipping and DEPRECATE_EOF #1708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis set of changes introduces explicit handling and negotiation of both MySQL and MariaDB extended client/server capabilities throughout the connection, handshake, and statement execution process. The capability flag system is refactored to distinguish between standard and extended flags, with new constants and types for MariaDB-specific features. Handshake packet parsing and response construction are updated to accommodate these changes, including conditional logic for TLS enforcement and metadata caching. Various methods for reading and skipping protocol packets are refactored for improved protocol compliance. Additionally, a benchmark for metadata-heavy queries is added, and tests are updated to validate the new handshake parsing logic. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Connector
participant MySQLConn
participant Server
App->>Connector: Connect()
Connector->>MySQLConn: readHandshakePacket()
MySQLConn->>Server: Receive handshake
MySQLConn-->>Connector: authData, serverCapabilities, serverExtCapabilities, plugin
Connector->>MySQLConn: initCapabilities(serverCapabilities, serverExtCapabilities, cfg)
Connector->>MySQLConn: writeHandshakeResponsePacket(authResp, plugin)
MySQLConn->>Server: Send handshake response
Server-->>MySQLConn: Authentication result
MySQLConn-->>Connector: Connection established
Connector-->>App: Return connection
sequenceDiagram
participant App
participant Stmt
participant MySQLConn
participant Server
App->>Stmt: Query()
Stmt->>MySQLConn: Send query
MySQLConn->>Server: Query execution
Server-->>MySQLConn: Result set header
MySQLConn-->>Stmt: columns, metadataFollows
alt metadataFollows
MySQLConn->>Server: Read columns
Stmt->>Stmt: Cache columns
else
Stmt->>Stmt: Use cached columns
end
MySQLConn->>Server: Stream rows
Stmt-->>App: Rows
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (9)
const.go (1)
76-86
: Well-structured implementation of MariaDB extended capabilities.The introduction of
extendedCapabilityFlag
type and associated constants properly encapsulates MariaDB-specific extended capabilities, particularly theclientCacheMetadata
flag that enables metadata skipping functionality.Consider adding a brief comment for each extended capability flag to document their purpose, especially for the
clientCacheMetadata
flag which is central to this PR's implementation.rows.go (1)
159-159
: Updated function call to handle new metadata caching information.The function call now correctly unpacks the additional boolean return value from
readResultSetHeaderPacket()
which likely indicates metadata caching presence.The unused variable (represented by
_
) indicates metadata caching information that's not needed in this context. Consider adding a brief comment explaining why this information is discarded here but might be used elsewhere.auth_test.go (1)
137-1346
: Comprehensive test updates for authentication methods with capability flags.All authentication-related tests have been consistently updated to match the new function signature, passing zero values for capability flags which is appropriate for these tests since they're not testing capability negotiation specifically.
Consider adding at least one test case that uses non-zero values for capability flags to explicitly test how capability negotiation affects the authentication process, particularly for metadata caching.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 293-293: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add
MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 665-665: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add
MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 868-868: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add
MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 1301-1301: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add
MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
benchmark_test.go (2)
402-407
: Populateargs
slice from index 0 to keep invariant obviousBecause the first
for
starts at index 1,args[0]
is assigned only in the nested loop below. The code is correct, but the two-phase “odd indices first, even indices later” pattern is a bit non-obvious to future readers.- for i := 1; i < 200; i += 2 { + for i := 0; i < 200; i += 2 {…and swap the later writer loop to fill the odd indices. Functionality is unchanged while intent becomes clearer.
459-512
: BenchmarkReceiveMetadata: tighten correctness & reduce noise
db.SetMaxIdleConns(0)
immediately followed bydb.SetMaxIdleConns(1)
cancels itself.rows.Next()
’s boolean return is ignored; if the single row is unexpectedly absent,Scan
triggers “row.Scan called without Next”.- The benchmark never checks
rows.Err()
; silent I/O errors could be missed.- db.SetMaxIdleConns(0) - db.SetMaxIdleConns(1) + db.SetMaxIdleConns(1) // one stable idle conn is enough - rows := tb.checkRows(stmt.Query()) - rows.Next() - // Scan the row - err := rows.Scan(valuePtrs...) - tb.check(err) - rows.Close() + rows := tb.checkRows(stmt.Query()) + if !rows.Next() { + rows.Close() + b.Fatalf("unexpectedly empty result set") + } + tb.check(rows.Scan(valuePtrs...)) + tb.check(rows.Close()) + tb.check(rows.Err())These tweaks make the benchmark more robust without affecting the performance signal.
connector.go (1)
133-143
: TLS enforcement check is welcome – but message could be clearerThe error text is surfaced to application code; clarifying that the server capability flags are missing helps operators:
- return nil, fmt.Errorf("TLS is required, but server doesn't support it") + return nil, fmt.Errorf("TLS requested in DSN but server did not advertise CLIENT_SSL capability")Minor wording tweak improves diagnosability.
packets.go (3)
282-318
: *The helper receives a Config but then ignores it
initClientCapabilities
takescfg *Config
, yet most of the checks (compress
,TLS
,MultiStatements
,DBName
) usemc.cfg
directly. Either:
- Drop the parameter and rely on
mc.cfg
, or- Use the
cfg
that is passed in to avoid confusion.-func (mc *mysqlConn) initClientCapabilities(serverCapabilities capabilityFlag, cfg *Config) capabilityFlag { +func (mc *mysqlConn) initClientCapabilities(serverCapabilities capabilityFlag) capabilityFlag { ... - if cfg.ClientFoundRows { + if mc.cfg.ClientFoundRows { ... - if cfg.compress { + if mc.cfg.compress { ... - if n := len(cfg.DBName); n > 0 { + if n := len(mc.cfg.DBName); n > 0 {This will also simplify the call-site (
mc.initClientCapabilities(serverCapabilities)
).
573-604
: Variable name ‑len
shadows the built-in
Usinglen
as a local variable hides the predeclaredlen()
function and can trip up future readers (and some IDE/go-analysis tooling).- num, _, len := readLengthEncodedInteger(data) + num, _, bytesRead := readLengthEncodedInteger(data) ... - return int(num), data[len] == 0x01, nil + return int(num), data[bytesRead] == 0x01, nilNo behavioural change – just clearer code.
921-928
: Blindly discarding a packet may swallow protocol errors
skipEof
reads (and discards) one packet whenclientDeprecateEOF
is not set but never checks that the packet is actually an EOF/OK packet. A malformed server response would pass unnoticed.-if mc.clientCapabilities&clientDeprecateEOF == 0 { - if _, err := mc.readPacket(); err != nil { - return err - } +if mc.clientCapabilities&clientDeprecateEOF == 0 { + pkt, err := mc.readPacket() + if err != nil { + return err + } + if pkt[0] != iEOF && pkt[0] != iOK { + return fmt.Errorf("protocol error: expected EOF/OK, got 0x%02x", pkt[0]) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
auth_test.go
(14 hunks)benchmark_test.go
(3 hunks)connection.go
(5 hunks)connector.go
(3 hunks)const.go
(2 hunks)packets.go
(15 hunks)packets_test.go
(1 hunks)rows.go
(3 hunks)statement.go
(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
connection.go (1)
dsn.go (1)
Config
(37-84)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (ubuntu-latest, 1.23, 9.0)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.11)
- GitHub Check: test (ubuntu-latest, 1.24, 9.0)
- GitHub Check: test (macos-latest, 1.24, 9.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: test (ubuntu-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, 8.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
🔇 Additional comments (14)
const.go (2)
46-46
: Improved type name enhances code clarity.Renaming
clientFlag
tocapabilityFlag
better reflects its purpose as it represents capabilities of both client and server, not just client flags.
49-49
: Renamed constant maintains consistent naming with its type.Renaming
clientLongPassword
toclientMySQL
aligns with the type name change and better represents its meaning as the first capability flag.packets_test.go (2)
335-335
: Updated function call signature to match implementation changes.The test correctly adapts to the modified
readHandshakePacket
function signature that now returns additional capability flags.
340-346
: Good test coverage for server capabilities.The added assertions verify that both standard and extended server capabilities are correctly parsed from the handshake packet, which is essential for testing the MariaDB metadata caching feature.
rows.go (2)
116-116
: More precise method for skipping result set rows.Replacing
readUntilEOF()
withskipResultSetRows()
improves code semantics and likely handles metadata caching more efficiently when closing rows.
146-146
: Consistent replacement of EOF reading with specialized method.This change aligns with the previous one, ensuring consistent handling of result sets with potential metadata caching.
auth_test.go (1)
92-92
: Updated function signature for handshake response packet.The call to
writeHandshakeResponsePacket
now includes the server capability parameters needed for capability negotiation during authentication.benchmark_test.go (1)
132-147
: Minor loop-formatting change is fineOnly whitespace was touched here; functionality and performance stay identical.
statement.go (2)
120-131
: Skip-EOF logic may mis-handle MariaDB “metadata skipped” pathWhen
metadataFollows == false
, MariaDB does not send an EOF packet; the first packet after the header is already the first row. Callingmc.skipEof()
risks consuming that row and corrupting the stream.Please verify with the MariaDB protocol docs; if confirmed, gate the EOF skip:
- if err = mc.skipEof(); err != nil { + if mc.mc.isMySQL() { // or similar capability check + if err = mc.skipEof(); err != nil { return nil, err } }Failing to do so will surface as “invalid sequence” or “unexpected field count” on the next read.
73-83
: Exec: fast-path skips — good changeReplacing
readUntilEOF
with explicitskipColumns
andskipResultSetRows
avoids parsing work and aligns with capability-aware handling. Nice!connector.go (2)
160-161
: HandshakeResponse now forwards server capability info – goodForward-propagating both capability bitsets keeps the client/server negotiation symmetric and maintainable.
174-177
: Compression gating uses negotiated client flags – correctSwitching from
mc.flags
tomc.clientCapabilities
fixes false-negatives when the initial capability mask differed. Looks good.connection.go (1)
223-242
: Conditional metadata path looks solid – just ensure compatibility downstream
The new branch that caches column metadata whenclientCacheMetadata
is negotiated is well-structured and will save network round-trips. Please double-check every call-site that later inspectsstmt.columns
; when metadata is skipped, it will now benil
, which may not have been the case before.packets.go (1)
945-960
: EOF detection may mis-classify large data packets
skipResultSetRows
treats any packet whose first byte is0xFE
and whose total length < 16 MB (0xffffff
) as EOF/OK. According to the protocol, row packets can also legitimately start with0xFE
when the first column’s length-encoded string happens to use the 0xFE marker and the row is small (< 8 bytes/field).A safer check is “length < 9” (EOF packet’s minimum size) which MySQL itself and other clients use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packets.go (1)
366-366
: Ineffectual assignment topos
variableThe increment to
pos
at line 366 is ineffectual as it's immediately overwritten. This was flagged by the static analysis tool.- pos += 4
🧰 Tools
🪛 golangci-lint (1.64.8)
366-366: ineffectual assignment to pos
(ineffassign)
🪛 GitHub Check: CodeQL
[warning] 366-366: Useless assignment to local variable
This definition of pos is never used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
connection.go
(5 hunks)connector.go
(3 hunks)const.go
(2 hunks)packets.go
(14 hunks)rows.go
(3 hunks)statement.go
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- rows.go
- statement.go
- const.go
- connection.go
- connector.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
packets.go
366-366: ineffectual assignment to pos
(ineffassign)
🪛 GitHub Check: CodeQL
packets.go
[warning] 366-366: Useless assignment to local variable
This definition of pos is never used.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test (ubuntu-latest, 1.22, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (macos-latest, 1.24, 9.0)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, 5.7)
🔇 Additional comments (27)
packets.go (27)
187-189
: Updated documentation references to include MariaDBGood addition of MariaDB protocol documentation link. This helps developers understand both MySQL and MariaDB protocol variations, which is important for the metadata skipping feature implementation.
189-190
: Function signature updated to support MariaDB capabilitiesThe function now returns both standard capabilities and MariaDB extended capabilities separately, which is necessary for properly implementing the metadata skipping feature.
196-197
: Error return handling aligned with updated function signatureCorrectly updated the return values in the error handling path to match the new function signature.
202-206
: Error return aligned with updated function signatureThe error return now properly includes the nil values for capabilities and empty plugin string, matching the new function signature.
220-223
: Updated capability flag handlingThe code now correctly parses capability flags from the handshake packet and returns early with appropriate error messages if required protocol features aren't supported.
224-229
: TLS requirement check updatedUpdated error return values to match the new function signature while maintaining the existing TLS verification logic.
238-247
: Added MariaDB extended capabilities parsingGood implementation for detecting and parsing MariaDB extended capabilities. The code checks if the server is not MySQL (
capabilities&clientMySQL == 0
) before attempting to read MariaDB extended capabilities.
275-276
: Updated return values for successful pathReturn values now include the extended capabilities parameter, matching the updated function signature.
281-282
: Updated minimal return valuesFinal return path correctly includes both standard and extended capability flags, with extended capabilities set to 0 in this minimal case.
284-322
: Well-structured new function for capability initializationThe new
initCapabilities
function provides a clean way to initialize client capabilities based on server support and configuration. It properly:
- Sets standard MySQL capabilities based on configuration
- Filters capabilities by what the server supports
- Sets MariaDB extended capabilities when applicable
This helps ensure proper capability negotiation between client and server.
324-373
: Improved handshake response packet constructionThe updated function now:
- Correctly uses the initialized capabilities
- Handles MariaDB extended capabilities separately
- Writes the appropriate filler based on server type
- Uses the correct packet format based on server capabilities
This provides proper support for both MySQL and MariaDB servers.
🧰 Tools
🪛 golangci-lint (1.64.8)
366-366: ineffectual assignment to pos
(ineffassign)
🪛 GitHub Check: CodeQL
[warning] 366-366: Useless assignment to local variable
This definition of pos is never used.
375-376
: Added documentation references for SSL connection requestGood addition of both MySQL and MariaDB protocol documentation links for the SSL connection request packet.
379-380
: Fixed packet writing in SSL requestCorrectly writes the packet data structure for SSL connection request.
394-418
: Improved packet construction using append operationsThe packet construction now uses append operations instead of fixed buffer copying, which is more flexible and less error-prone, especially when dealing with variable-length data like user, auth data, database name, plugin name, and connection attributes.
421-422
: Simplified return statementRefactored to directly return the result of
mc.writePacket(data)
which is cleaner.
556-557
: Function signature updated to return metadata presence flagThe function now returns an additional boolean to indicate whether metadata is present, which is essential for the metadata skipping feature.
563-564
: Error return aligned with updated function signatureError handling correctly updated to return the additional boolean parameter.
568-574
: Updated return statements to include metadata flagAll return paths now correctly include the metadata presence flag parameter.
579-587
: Implemented metadata presence detectionThe code now correctly:
- Parses the metadata presence flag for MariaDB servers
- Returns the appropriate boolean value based on client capabilities
- Returns true by default for MySQL servers (which always send metadata)
This is a key part of the metadata skipping feature implementation.
710-788
: Fixed column reading loop to handle exact countThe column reading loop now iterates exactly
count
times instead of looking for an EOF packet, making it more robust and consistent with the protocol specification. This change supports the metadata skipping feature by allowing precise column count handling.
790-794
: Added EOF packet handling for column definitionThe function now correctly skips the EOF packet if the client doesn't support
deprecateEOF
. This is important for protocol compliance and backward compatibility.
812-831
: Enhanced EOF packet detection logic in text rowsThe code now properly:
- Handles the edge case where 0xFE can be part of a length-encoded string
- Distinguishes between EOF packets and OK packets with 0xFE header
- Parses status flags from the appropriate location based on packet type
This improves protocol compliance and robustness.
899-915
: Added helper functions for packet skippingThe new
skipPackets
andskipEof
functions provide clean abstractions for common packet handling operations, reducing code duplication and improving maintainability.
917-923
: Added helper function for skipping columnsThe
skipColumns
function combines skipping a specific number of packets with skipping the EOF packet, providing a higher-level abstraction for a common operation in the codebase.
925-952
: Enhanced row skipping logicThe
skipRows
function now correctly:
- Detects the end of rows (EOF or OK packet)
- Handles both deprecated and modern protocol variants
- Updates connection status appropriately
This improves protocol compliance and supports the metadata skipping feature.
1236-1253
: Updated result discarding to use helper functionsThe
discardResults
method now uses the new helper functions to skip columns and rows, making the code more maintainable and consistent.
1264-1280
: Enhanced EOF packet detection logic in binary rowsSimilar to the text rows, the binary rows handler now properly distinguishes between EOF packets and OK packets with 0xFE header, and parses status flags accordingly.
@@ -156,7 +156,7 @@ func (rows *mysqlRows) nextResultSet() (int, error) { | |||
rows.rs = resultSet{} | |||
// rows.mc.affectedRows and rows.mc.insertIds accumulate on each call to | |||
// nextResultSet. | |||
resLen, err := rows.mc.resultUnchanged().readResultSetHeaderPacket() | |||
resLen, _, err := rows.mc.resultUnchanged().readResultSetHeaderPacket() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need cache columns in multi resultset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the rare case where there is a multi result-set using binary protocol, metadata will always be sent, it goes against the goal of metadata skipping (to save the list of columns per prepare).
so to answer, no need
@rusher Please review my commits. |
if mc.cfg.TLS != nil && serverCapabilities&clientSSL == 0 { | ||
return nil, fmt.Errorf("TLS is required, but server doesn't support it") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch
Implement MariaDB metadata skipping
This is in replacement for #1692, for a simplier (still requiring lots of changes).
See MariaDB metadata skipping. With this change, MariaDB server won't send metadata when they have not changed, saving client parsing metadata and network.
Even if metadata skipping change is not so big, this rely on big changes :
A benchmark BenchmarkReceiveMetadata has been added to show the difference.
On a local server, difference gain is only a few percent.
on a distant server, difference is huge. Example here with a server next to client :
goos: linux
goarch: amd64
pkg: github.com/go-sql-driver/mysql
cpu: 11th Gen Intel(R) Core(TM) i9-11900K @ 3.50GHz
BenchmarkReceiveMetadata
BenchmarkReceiveMetadata/query
before :
BenchmarkReceiveMetadata/query-16 138 7849971 ns/op 90379 B/op 1015 allocs/op
after change:
BenchmarkReceiveMetadata/query-16 362 2974155 ns/op 33337 B/op 16 allocs/op
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests